[css-grid] Fix resolution of percentage paddings and margins of grid items #10194
Conversation
Build ERROREDStarted: 2018-04-02 10:40:15 Failing Jobs
Unstable ResultsBrowser: "Chrome Dev"View in: WPT PR Status | TravisCI
|
chromium-wpt-export-bot
changed the title
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-980756
branch
from
7f23119
to
0a619a0
Compare
…items We were not resolving properly percentage paddings and margins for tracks that have something like minmax(auto, 100px). The reason was that while computing the minimum size of a grid item, the percentages were resolved against the inline size of the grid container. But for grid items we shouldn't never use the grid container size, but the grid area size, as that's their containing block. The patch modifies ContainingBlockLogicalWidthForContent() and ContainingBlockLogicalHeightForContent() in LayoutBox, so for grid items we return 0 if the area size hasn't been set yet. We never want to use the grid container's sizes in these cases. BUG=808758 TEST=external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-* TEST=external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-* Change-Id: Ib142e51aee1fe623d38688469b179f01f82eb07b Reviewed-on: https://chromium-review.googlesource.com/980756 Reviewed-by: Javier Fernandez <[email protected]> Commit-Queue: Manuel Rego Casasnovas <[email protected]> Cr-Commit-Position: refs/heads/master@{#547417}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-980756
branch
from
0a619a0
to
687aa08
Compare
|
@mrego it looks like some of these tests are flaky on Chrome Dev and this is what's blocking the PR from being merged. Can you check if you're able to reproduce the flakiness? From the Travis logs it looks like maybe a layout hasn't happened that the test assumes has happened. |
|
@foolip I managed to run the tests locally (thanks @Ms2ger) with: and also: But I cannot reproduce the flakiness locally. In Chromium we have some issues with the percentage resolutions and results vary depending if the layout phase is run once, twice, etc. The patch is fixing some of them (there might other things still present). What should we do regarding these tests? PD: Sorry for not realizing about this Travis issue before merging the upstream patch (it'd be awesome that Travis somehow notifies the Chromium CL to avoid this kind of things). |
|
I managed to reproduce the flakiness locally with a reduced test case, and I can confirm the patch that has just landed on Chromium fixes it. |
We want this as extension of #7475 (@lukebjerring FYI) but as made evident by this issue it's a bit tricky. Testing something that's just a little different from and a little bit older than the content_shell built and tested can get confusing. One option might be to also test on using chrome+chromedriver built from the Chromium CQ and ignore failures that are only in Chrome Dev, but that'd also decrease the odds of detecting real flakiness. @mrego, until we have this figured out, you don't need to worry about things grong wrong in Travis, I'd wager that more often than not no change to the original CL is needed, i.e., the signal:noise ratio isn't high enough with our current setup. |
We were not resolving properly percentage paddings and margins
for tracks that have something like minmax(auto, 100px).
The reason was that while computing the minimum size of a grid item,
the percentages were resolved against the inline size of the grid container.
But for grid items we shouldn't never use the grid container size,
but the grid area size, as that's their containing block.
The patch modifies ContainingBlockLogicalWidthForContent() and
ContainingBlockLogicalHeightForContent() in LayoutBox,
so for grid items we return 0 if the area size hasn't been set yet.
We never want to use the grid container's sizes in these cases.
BUG=808758
TEST=external/wpt/css/css-grid/grid-items/grid-items-percentage-margins-*
TEST=external/wpt/css/css-grid/grid-items/grid-items-percentage-paddings-*
Change-Id: Ib142e51aee1fe623d38688469b179f01f82eb07b
Reviewed-on: https://chromium-review.googlesource.com/980756
Reviewed-by: Javier Fernandez [email protected]
Commit-Queue: Manuel Rego Casasnovas [email protected]
Cr-Commit-Position: refs/heads/master@{#547417}